Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Use ranges to define versions of dependencies for converged components #17499

Merged
merged 1 commit into from
May 12, 2021

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Mar 19, 2021

This RFC proposes to continue use caret (^) and tilde (~) to define version ranges for production dependencies of converged packages.

Please check rendered preview.

@layershifter layershifter changed the title RFC: Use ranges to define versions of dependencies RFC: Use ranges to define versions of dependencies for converged components Mar 19, 2021
@layershifter layershifter added the Type: RFC Request for Feedback label Mar 19, 2021
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ffab59c:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@size-auditor
Copy link

size-auditor bot commented Mar 19, 2021

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: c49525b95c315995c323e2b7efe50c637ceb539e (build)

@fabricteam
Copy link
Collaborator

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1164 1213 5000
BaseButton mount 988 958 5000
Breadcrumb mount 44114 45078 5000
ButtonNext mount 1361 1331 5000
Checkbox mount 1679 1641 5000
CheckboxBase mount 1378 1381 5000
ChoiceGroup mount 4946 4935 5000
ComboBox mount 1036 1044 1000
CommandBar mount 10324 10304 1000
ContextualMenu mount 6370 6389 1000
DefaultButton mount 1176 1217 5000
DetailsRow mount 3835 3754 5000
DetailsRowFast mount 3797 3811 5000
DetailsRowNoStyles mount 3590 3683 5000
Dialog mount 1534 1544 1000
DocumentCardTitle mount 1836 1845 1000
Dropdown mount 3594 3538 5000
FocusTrapZone mount 1849 1852 5000
FocusZone mount 1834 1796 5000
IconButton mount 1926 1895 5000
Label mount 340 360 5000
Layer mount 1870 1851 5000
Link mount 486 488 5000
MakeStyles mount 1963 1942 50000
MenuButton mount 1585 1554 5000
MessageBar mount 2006 2099 5000
Nav mount 3365 3506 1000
OverflowSet mount 1025 1043 5000
Panel mount 1455 1528 1000
Persona mount 851 817 1000
Pivot mount 1466 1503 1000
PrimaryButton mount 1376 1344 5000
Rating mount 8080 8166 5000
SearchBox mount 1437 1402 5000
Shimmer mount 2719 2631 5000
Slider mount 2037 2013 5000
SpinButton mount 5207 5117 5000
Spinner mount 454 461 5000
SplitButton mount 3387 3294 5000
Stack mount 515 525 5000
StackWithIntrinsicChildren mount 1610 1673 5000
StackWithTextChildren mount 4845 4939 5000
SwatchColorPicker mount 10687 10646 5000
Tabs mount 1465 1448 1000
TagPicker mount 2963 2893 5000
TeachingBubble mount 11887 11828 5000
Text mount 452 432 5000
TextField mount 1484 1463 5000
ThemeProvider mount 1190 1188 5000
ThemeProvider virtual-rerender 613 594 5000
ThemeProviderNext mount 15741 15606 5000
Toggle mount 828 831 5000
buttonNative mount 118 118 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🦄 Avatar.Fluent 0.2 0.51 0.39:1 2000 392
🦄 Button.Fluent 0.14 0.22 0.64:1 5000 676
🔧 Checkbox.Fluent 0.71 0.38 1.87:1 1000 708
🎯 Dialog.Fluent 0.17 0.23 0.74:1 5000 863
🔧 Dropdown.Fluent 3.33 0.44 7.57:1 1000 3329
🔧 Icon.Fluent 0.15 0.06 2.5:1 5000 773
🦄 Image.Fluent 0.09 0.14 0.64:1 5000 461
🔧 Slider.Fluent 1.64 0.48 3.42:1 1000 1638
🔧 Text.Fluent 0.09 0.03 3:1 5000 433
🦄 Tooltip.Fluent 0.15 0.93 0.16:1 5000 771

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AvatarMinimalPerf.default 241 216 1.12:1
SkeletonMinimalPerf.default 458 418 1.1:1
ButtonMinimalPerf.default 222 205 1.08:1
ChatDuplicateMessagesPerf.default 326 303 1.08:1
TreeWith60ListItems.default 210 196 1.07:1
GridMinimalPerf.default 419 394 1.06:1
RefMinimalPerf.default 261 247 1.06:1
StatusMinimalPerf.default 856 805 1.06:1
Checkbox.Fluent 708 671 1.06:1
Icon.Fluent 773 728 1.06:1
ButtonUseCssPerf.default 932 886 1.05:1
ListNestedPerf.default 678 645 1.05:1
ListWith60ListItems.default 740 705 1.05:1
PortalMinimalPerf.default 174 165 1.05:1
RadioGroupMinimalPerf.default 539 513 1.05:1
TableMinimalPerf.default 496 472 1.05:1
TooltipMinimalPerf.default 1074 1027 1.05:1
CardMinimalPerf.default 661 634 1.04:1
ListCommonPerf.default 768 741 1.04:1
ReactionMinimalPerf.default 471 452 1.04:1
Avatar.Fluent 392 377 1.04:1
Text.Fluent 433 415 1.04:1
ButtonOverridesMissPerf.default 1905 1846 1.03:1
CarouselMinimalPerf.default 543 527 1.03:1
DividerMinimalPerf.default 441 430 1.03:1
FlexMinimalPerf.default 354 344 1.03:1
PopupMinimalPerf.default 757 737 1.03:1
ToolbarMinimalPerf.default 1098 1066 1.03:1
ButtonUseCssNestingPerf.default 1212 1193 1.02:1
DatepickerMinimalPerf.default 50336 49546 1.02:1
DropdownMinimalPerf.default 3267 3204 1.02:1
FormMinimalPerf.default 517 507 1.02:1
ItemLayoutMinimalPerf.default 1395 1364 1.02:1
ProviderMinimalPerf.default 1028 1006 1.02:1
TextMinimalPerf.default 425 418 1.02:1
TreeMinimalPerf.default 919 899 1.02:1
Button.Fluent 676 660 1.02:1
Dropdown.Fluent 3329 3262 1.02:1
AttachmentSlotsPerf.default 1360 1343 1.01:1
DialogMinimalPerf.default 885 877 1.01:1
DropdownManyItemsPerf.default 819 812 1.01:1
HeaderMinimalPerf.default 456 450 1.01:1
ListMinimalPerf.default 567 563 1.01:1
LoaderMinimalPerf.default 805 795 1.01:1
SplitButtonMinimalPerf.default 4165 4112 1.01:1
Tooltip.Fluent 771 761 1.01:1
EmbedMinimalPerf.default 4637 4636 1:1
ImageMinimalPerf.default 462 462 1:1
MenuButtonMinimalPerf.default 1799 1798 1:1
SegmentMinimalPerf.default 430 429 1:1
TextAreaMinimalPerf.default 576 578 1:1
CustomToolbarPrototype.default 4009 4010 1:1
Image.Fluent 461 462 1:1
AnimationMinimalPerf.default 459 465 0.99:1
ChatWithPopoverPerf.default 439 442 0.99:1
CheckboxMinimalPerf.default 3062 3089 0.99:1
MenuMinimalPerf.default 1003 1015 0.99:1
ProviderMergeThemesPerf.default 1648 1670 0.99:1
TableManyItemsPerf.default 2357 2385 0.99:1
Slider.Fluent 1638 1654 0.99:1
LabelMinimalPerf.default 483 495 0.98:1
SliderMinimalPerf.default 1682 1722 0.98:1
Dialog.Fluent 863 885 0.98:1
ChatMinimalPerf.default 700 719 0.97:1
HeaderSlotsPerf.default 914 940 0.97:1
VideoMinimalPerf.default 705 726 0.97:1
AccordionMinimalPerf.default 184 191 0.96:1
AlertMinimalPerf.default 354 367 0.96:1
ButtonSlotsPerf.default 617 644 0.96:1
InputMinimalPerf.default 1374 1430 0.96:1
AttachmentMinimalPerf.default 189 198 0.95:1
IconMinimalPerf.default 751 792 0.95:1
BoxMinimalPerf.default 399 428 0.93:1
LayoutMinimalPerf.default 476 510 0.93:1
RosterPerf.default 1202 1305 0.92:1


Use semver ranges to define production dependencies for our customers. Exact versions can be used in non-distributed code (build and infra tools).

_Optionally_: Distribute with packages `VERSIONS` file that will contain versions of dependencies that are used on our side in machine readable format that can be potentially used by customers in their pipelines.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not be too hasty, as I said will follow up with OSS CELA on the actual requirements ASAP

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CELA conclusion: we can only run enhanced license scan on specific versions we use, so this idea could work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "this idea" do you mean the VERSIONS file? How would that work from a consumer's perspective, specifically re: legal? (Feel free to respond on teams if needed.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial idea was that we can implement a Webpack plugin that will ensure that included dependencies match versions defined in VERSIONS file.

@paulgildea paulgildea requested a review from ecraig12345 March 31, 2021 16:45
Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me in general but I'd appreciate clarification on a couple nuances.


Thus a single package gets included multiple times in a Webpack bundle due to different package versions. This situation will happen without any warning, resulting in extra bloat in consumer's bundle and may lead to hard-to-find bugs.

It's also harder for consumers in this case to consume security patches, for example `lodash@4.17.20` contains [a security vulnerability](https://snyk.io/vuln/SNYK-JS-LODASH-1018905) but this version will be inside consumer's application without additional movements from their side. If we will use a caret ("lodash": "^4.17.20") only a single version (a latest in a range) will be installed:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we will use a caret ("lodash": "^4.17.20") only a single version (a latest in a range) will be installed

Just a heads up, this is not necessarily true--if a dep on lodash@^4.17.20 resolved to 4.17.20 already exists in the lockfile, and that semver spec is still referenced by another package, yarn may add a separate entry for lodash@^4.17.21 => 4.17.21 instead of resolving both to 4.17.21.

(Doesn't affect the proposal since this is an issue with yarn in general, but I wanted to call it out for awareness.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're totally right 👍 But with ranges we can at least deduplicate dependencies manually or via tools (yarn-deduplicate for Yarn v1 or yarn dedupe for V2).


## Detailed Design or Proposal

Use semver ranges to define production dependencies for our customers. Exact versions can be used in non-distributed code (build and infra tools).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple possible issues/nuances here that I wonder if we should include in the proposal re: different versions for production and dev dependencies:

  1. How do we set this up with syncpack? My first idea would be something like this (supposing converged1 and converged2 depend on lodash@^4.17.21, whereas other packages have a specific version in devDependencies):
{
  "packages": ["@fluentui/converged1", "@fluentui/converged2"],
  "dependencies": ["lodash"]
}
  1. How do we guarantee that yarn resolves the production dep to the same version as the dev dep within our repo? Without tooling enforcement, it's entirely possible that yarn could choose to resolve the two versions differently. My first idea (which probably won't work, see below) is this. (The leading ** is necessary to make resolutions work for packages within the workspace.)
{
  "resolutions": {
    "**/@fluentui/*/lodash": "4.17.21"
  }
}

However this might not work because the resolutions spec says a single * is not supported (would probably be good to test it in case the implementation is different). **/@fluentui/**/lodash should be supported, but that would resolve all nested tools' lodash deps to a specific version, which might be a problem. So we'd probably have to figure out some other approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we set this up with syncpack?

Do you mean our repo, correct? Can please expand a bit more there, I did not get the problem.

How do we guarantee that yarn resolves the production dep to the same version as the dev dep within our repo?

IMO we should not guarantee this, for example: we don't restrict people to use the same version of React as in our repo, why it should be different for other dependencies? It means that we should be accurate in choosing dependencies, they should follow semver.

However, resolutions can be and should be used by customers for legal issues. I tried your approach with resolutions on Yarn v1:

{
  "dependencies": {
    "@fluentui/react-components": "^9.0.0-alpha.36",
    "@popperjs/core": "^2.9.2"
  },
  "resolutions": {
    "**/@fluentui/*/@popperjs/core": "2.8.0"
  }
}

This results in two versions installed:

"@popperjs/core@2.8.0", "@popperjs/core@~2.4.3":
  version "2.8.0"

"@popperjs/core@^2.9.2":
  version "2.9.2"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we set this up with syncpack?

Do you mean our repo, correct? Can please expand a bit more there, I did not get the problem.

With syncpack you're only allowed to depend on one semver spec for a given dep, unless you set up a versionGroups entry allowing certain packages to use a different version spec for that dep.

How do we guarantee that yarn resolves the production dep to the same version as the dev dep within our repo?

IMO we should not guarantee this, for example: we don't restrict people to use the same version of React as in our repo, why it should be different for other dependencies? It means that we should be accurate in choosing dependencies, they should follow semver.

I meant within our repo, not from a customer standpoint. Yarn may sometimes decide to resolve foo@^1.0.0 (production dep) and foo@1.1.0 (dev dep) differently, resulting in duplicates within our build process, which is a significant part of the problem @Hotell was trying to resolve to begin with.

The resolutions suggestion was also meant to be used within our repo. (If it doesn't work as I wrote it, we could do some more experimenting.)

Copy link
Member Author

@layershifter layershifter May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ecraig12345 so this thread is related only to our repo correct?

With syncpack you're only allowed to depend on one semver spec for a given dep, unless you set up a versionGroups entry allowing certain packages to use a different version spec for that dep.

As I know @Hotell proposed to move out all dev dependencies to the root package.json. If we will do this and ignore the root package.json in syncpack, will it solve the issue?

Yarn may sometimes decide to resolve foo@^1.0.0 (production dep) and foo@1.1.0 (dev dep) differently, resulting in duplicates within our build process, which is a significant part of the problem @Hotell was trying to resolve to begin with.

Honestly I don't think that I understand what solves usage of exact versions for dev dependencies.

  • we are sure that we are using an exact version, for example webpack@5.0.0
  • it does not solve duplicate dependencies as storybook may depend on webpack@^5.1.0
  • this results in two entries for webpack (and potentially its sub dependencies) in our yarn.lock

Are these statements correct?


Use semver ranges to define production dependencies for our customers. Exact versions can be used in non-distributed code (build and infra tools).

_Optionally_: Distribute with packages `VERSIONS` file that will contain versions of dependencies that are used on our side in machine readable format that can be potentially used by customers in their pipelines.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "this idea" do you mean the VERSIONS file? How would that work from a consumer's perspective, specifically re: legal? (Feel free to respond on teams if needed.)

@ecraig12345 ecraig12345 requested a review from Hotell April 1, 2021 01:51
@levithomason
Copy link
Member

Bump, sounds like next week we can close this up @layershifter?

@layershifter
Copy link
Member Author

Bump, sounds like next week we can close this up @layershifter?

Yes, merging this as I don't see blocking items. @ecraig12345 we can continue conversation on this PR or in offline 😉

@layershifter layershifter merged commit a1e6c83 into master May 12, 2021
@layershifter layershifter deleted the rfcs/dependency-versions branch May 12, 2021 12:09
@ecraig12345
Copy link
Member

ecraig12345 commented May 13, 2021

Yes, merging this as I don't see blocking items. @ecraig12345 we can continue conversation on this PR or in offline 😉

@layershifter I understand wanting to move things forward, but in the future I'd really appreciate direct confirmation (or a chance on business hours to see and respond to comments) before going ahead with merging despite outstanding comments. I didn't block the PR because I agree with the overall concept, but the problem of how to prevent duplicates locally is very important for maintaining reliable, predictable builds and dev experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: RFC Request for Feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants